-
Notifications
You must be signed in to change notification settings - Fork 325
fix: default max connections in cloudflare to lower #1024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
That's a compiled file, so the change will be removed on publish. |
|
Ah I see, sorry. Would it make sense to add this to the |
|
Let me know if you'd like to see any other changes here @porsager. It'd be good to get this merged to prevent this common footgun! |
4fd011e to
a92f470
Compare
4a0fe34 to
3a43815
Compare
|
Let me know if there's anything else I can do to get this landed @porsager, thanks! |
|
Hi James.. Yes - let's get this finished. I'm wondering if maybe dynamic resolution is better? If we at one point can move the Cloudflare polyfill I'm afraid we'll forget the max: 3 limit. Is there an env var we could rely on in cloudflare or perhaps a global? |
|
Cloudflare does make |
|
Something like this should work after the // Adjust max connections for Cloudflare Workers
// https://github.com/porsager/postgres/issues/1023
if (typeof globalThis !== 'undefined' && 'Cloudflare' in globalThis) {
defaults.max = 3;
}Happy to do that if preferred, let me know! |
|
Cool! globalThis is available on all supported platforms, so if you add it as a ternary to defaults like this instead that'd be great! max : globalThis.Cloudflare ? 3 : 10A nice and simple one line PR ;) |
|
Done, thanks! Let me know if any other changes are needed. |
|
Nice - just remove the changes to everything but src/index.js as those are built automatically anyway. Also remove the comment - git keeps enough history ;) |
5f3645f to
27b6370
Compare
|
Sounds good, done, thanks |
Fixes #1023